fix: Dedup duplicate crates with differing origins in CrateGraph construction #15754

alibektas · 2023-10-13T21:45:17Z

Partially fixes #15656 . Until now the condition for deduplication in crate graphs were the strict equality of two crates. One problem that arises from this is that in certain conditions when we see the same crate having different CrateOrigins the first occurrence would be kept. This approach however results in some unwanted results such as making renaming forbidden as this has been recently only made available for local crates. The given example in #15656 can still not be resolved with this PR as that involves taking inconsistencies between dependencies into consideration. This will be addressed in a future PR.

crates/base-db/src/input.rs

crates/cfg/src/lib.rs

crates/rust-analyzer/tests/slow-tests/main.rs

crates/base-db/src/input.rs

alibektas · 2023-11-11T09:45:21Z

Forgot to move dev deps from lib crate to the local, so please wait for the next commit before reviewing

alibektas · 2023-11-11T12:59:52Z

This is ready to be reviewed a third time

crates/base-db/src/input.rs

crates/cfg/src/lib.rs

crates/base-db/src/input.rs

Partially fixes rust-lang#15656 . When a crate graph is extended which is the case when new workspaces are added to the project the rules for deduplication were too strict. One problem that arises from this is that in certain conditions when we see the same crate having different `CrateOrigin`s the first form would be maintained. This approach however results in some unwanted results such as making renaming forbidden as this has been recently only made available for local crates. The given example in rust-lang#15656 can still not be resolved with this PR as that involves taking inconsistencies between dependencies into consideration. This will be addressed in a future PR.

Make data reflect a case where dev deps are existent. base-db::CrateGraph::extend now adds dev dependencies for a crate in case of its upgrading from a CrateOrigin::Lib kind of a crate to a CrateOrigin::Local one.

Veykril · 2023-11-23T09:38:37Z

@bors r+

bors · 2023-11-23T09:38:39Z

📌 Commit be91d95 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-11-23T09:39:47Z

⌛ Testing commit be91d95 with merge 49b8a8e...

… r=Veykril fix: Dedup duplicate crates with differing origins in CrateGraph construction Partially fixes #15656 . Until now the condition for deduplication in crate graphs were the strict equality of two crates. One problem that arises from this is that in certain conditions when we see the same crate having different `CrateOrigin`s the first occurrence would be kept. This approach however results in some unwanted results such as making renaming forbidden as this has been recently only made available for local crates. The given example in #15656 can still not be resolved with this PR as that involves taking inconsistencies between dependencies into consideration. This will be addressed in a future PR.

bors · 2023-11-23T09:48:50Z

💔 Test failed - checks-actions

lnicola · 2023-11-23T10:33:53Z

@bors retry

Veykril · 2023-11-23T10:34:19Z

That last commit contains nothing?

Veykril · 2023-11-23T10:35:00Z

(also retry only works when the PR history has not been changed since the last r+)

lnicola · 2023-11-23T10:35:56Z

Yeah, last commit is empty, but there was a force-push that did change some paths: https://github.com/rust-lang/rust-analyzer/compare/7dabf2718ef3724d12d9999c120d21df343253c6..32f382e4e8ea841da991ff3c3faade3e4d5052c5.

Veykril · 2023-11-23T11:06:54Z

@bors r+

bors · 2023-11-23T11:06:56Z

📌 Commit 736994f has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-11-23T11:08:04Z

⌛ Testing commit 736994f with merge 98fbe82...

… r=Veykril fix: Dedup duplicate crates with differing origins in CrateGraph construction Partially fixes #15656 . Until now the condition for deduplication in crate graphs were the strict equality of two crates. One problem that arises from this is that in certain conditions when we see the same crate having different `CrateOrigin`s the first occurrence would be kept. This approach however results in some unwanted results such as making renaming forbidden as this has been recently only made available for local crates. The given example in #15656 can still not be resolved with this PR as that involves taking inconsistencies between dependencies into consideration. This will be addressed in a future PR.

bors · 2023-11-23T11:17:46Z

💔 Test failed - checks-actions

lnicola · 2023-11-23T13:00:38Z

@bors r=Veykril

bors · 2023-11-23T13:00:41Z

📌 Commit ba1b080 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-11-23T13:01:48Z

⌛ Testing commit ba1b080 with merge cccc7ca...

bors · 2023-11-23T13:12:37Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing cccc7ca to master...

matklad · 2023-12-20T18:36:39Z

crates/base-db/src/input.rs

+    Normal,
+    Dev,
+    Build,
+}


Please don't take this too seriously, as I have retreated to my Zig ivory tower in Lisbon some years ago and am mostly out of touch with the current goings of rust-analyzer, but I would say this is a significant architectural bug.

By design, this layer of rust-analyzer knows nothing about cargo specific concepts. dev-vs-build-vs normal is 100% Cargo concept. rustc knows nothing about these words. As such, any special handling of these concepts should happen in workspace.rs, not here.

The motivation for this design is two-fold:

practically, Cargo is a build system, there might be others. In the core analysis parts, we shouldn't be prioritizing Cargo over other (current and future) build systems

theoretically, it is important that the internal objects in rust-analyzer's semantic model reflect the physical reality of rustc. In that physical reality, cargo packages do not exists (and -dev as a concept applies to a package), there are only units of compilation.

cc @Veykril

Please don't take this too seriously,

Not at all, you are completely right, this seems incorrect. This was a mistake on my part (I proposed this change somewhere I believe, might've been in DMs).

In order to check for equality we needed to filter out dev dependencies and thus we ended up with the current state of things. The alternatives were

To filter them in project_model : I am looking into this since @matklad 's first comment here, I do not think it is going to be easy.

Adding project-model as a dependency of base-db which is ofc super wrong to do. ( As a matter of fact we already do have a DependencyKind under project-model named DepKind )

But let me say that I also agree with matklad's concerns so I will look for a way to rectify this.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2023

alibektas changed the title ~~Relaxation for crate graph mergin~~ fix : relax dedup rules for crate graphs Oct 13, 2023

alibektas changed the title ~~fix : relax dedup rules for crate graphs~~ fix: relax dedup rules for crate graphs Oct 13, 2023

Veykril reviewed Oct 15, 2023

View reviewed changes

alibektas force-pushed the 15656/linked_projects_are_local_too branch 2 times, most recently from 91225bb to bf53935 Compare October 15, 2023 16:25

Veykril reviewed Oct 17, 2023

View reviewed changes

ogapo mentioned this pull request Nov 5, 2023

[Since 0.4.1658] Cannot rename a non-local definition. #15656

Closed

alibektas force-pushed the 15656/linked_projects_are_local_too branch from bf53935 to 1a9b845 Compare November 10, 2023 23:42

alibektas force-pushed the 15656/linked_projects_are_local_too branch from fe7f479 to c3f90bb Compare November 11, 2023 12:49

alibektas commented Nov 12, 2023

View reviewed changes

crates/base-db/src/input.rs Outdated Show resolved Hide resolved

Veykril reviewed Nov 21, 2023

View reviewed changes

crates/cfg/src/lib.rs Outdated Show resolved Hide resolved

crates/base-db/src/input.rs Outdated Show resolved Hide resolved

crates/base-db/src/input.rs Outdated Show resolved Hide resolved

Veykril reviewed Nov 21, 2023

View reviewed changes

crates/base-db/src/input.rs Outdated Show resolved Hide resolved

Veykril reviewed Nov 21, 2023

View reviewed changes

crates/base-db/src/input.rs Outdated Show resolved Hide resolved

Veykril reviewed Nov 21, 2023

View reviewed changes

crates/base-db/src/input.rs Outdated Show resolved Hide resolved

alibektas added 5 commits November 23, 2023 02:15

v2

7e4aad5

v3

25e990d

Update test data for crate deduping

74d8fdc

Make data reflect a case where dev deps are existent. base-db::CrateGraph::extend now adds dev dependencies for a crate in case of its upgrading from a CrateOrigin::Lib kind of a crate to a CrateOrigin::Local one.

v3

f79e818

alibektas force-pushed the 15656/linked_projects_are_local_too branch from ef9bc3e to be91d95 Compare November 23, 2023 01:46

Veykril approved these changes Nov 23, 2023

View reviewed changes

Veykril changed the title ~~fix: relax dedup rules for crate graphs~~ fix: Dedup duplicate crates with differing origins in CrateGraph construction Nov 23, 2023

alibektas force-pushed the 15656/linked_projects_are_local_too branch from 7dabf27 to 32f382e Compare November 23, 2023 10:23

Make test cases simpler

736994f

alibektas force-pushed the 15656/linked_projects_are_local_too branch from 32f382e to 736994f Compare November 23, 2023 10:54

Precede paths with $ROOT$

ba1b080

bors merged commit cccc7ca into rust-lang:master Nov 23, 2023
9 checks passed

Veykril mentioned this pull request Nov 27, 2023

No feedback from Rust Analyzer in multi-Cargo workspace VS Code workspace #15974

Closed

matklad reviewed Dec 20, 2023

View reviewed changes

matklad mentioned this pull request Dec 20, 2023

dev-dependencies on workspace member cause cyclic dependency issues #14167

Open

Veykril mentioned this pull request Dec 20, 2023

Remove accidental cargo knowledge from CrateGraph deduplication #16170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Dedup duplicate crates with differing origins in CrateGraph construction #15754

fix: Dedup duplicate crates with differing origins in CrateGraph construction #15754

alibektas commented Oct 13, 2023 •

edited

Loading

alibektas commented Nov 11, 2023

alibektas commented Nov 11, 2023

Veykril commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

lnicola commented Nov 23, 2023

Veykril commented Nov 23, 2023

Veykril commented Nov 23, 2023

lnicola commented Nov 23, 2023

Veykril commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

lnicola commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

matklad Dec 20, 2023

Veykril Dec 20, 2023

alibektas Dec 20, 2023

alibektas Dec 20, 2023

fix: Dedup duplicate crates with differing origins in CrateGraph construction #15754

fix: Dedup duplicate crates with differing origins in CrateGraph construction #15754

Conversation

alibektas commented Oct 13, 2023 • edited Loading

alibektas commented Nov 11, 2023

alibektas commented Nov 11, 2023

Veykril commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

lnicola commented Nov 23, 2023

Veykril commented Nov 23, 2023

Veykril commented Nov 23, 2023

lnicola commented Nov 23, 2023

Veykril commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

lnicola commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

matklad Dec 20, 2023

Choose a reason for hiding this comment

Veykril Dec 20, 2023

Choose a reason for hiding this comment

alibektas Dec 20, 2023

Choose a reason for hiding this comment

alibektas Dec 20, 2023

Choose a reason for hiding this comment

alibektas commented Oct 13, 2023 •

edited

Loading